Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix cyclic compilation: Use vendored once_cell #1154

Merged
merged 5 commits into from
Jul 15, 2024
Merged

Fix cyclic compilation: Use vendored once_cell #1154

merged 5 commits into from
Jul 15, 2024

Conversation

NobodyXu
Copy link
Collaborator

Fix #1146

@NobodyXu NobodyXu requested review from ChrisDenton, thomcc and workingjubilee and removed request for ChrisDenton July 13, 2024 08:21
workingjubilee
workingjubilee requested changes Jul 13, 2024
View reviewed changes
Copy link
Member

@workingjubilee workingjubilee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer to use the OnceLock terminology from std if we're going to use a Sync type, because I have already gotten used to the Cell vs. Lock distinction in my personal usage of these APIs.

src/parallel/once_cell.rs Outdated Show resolved Hide resolved
src/parallel/job_token.rs Outdated Show resolved Hide resolved
src/parallel/job_token.rs Outdated Show resolved Hide resolved
@thomcc
Copy link
Member

thomcc commented Jul 14, 2024

I'm requested as a reviewer, but I'll leave my review to @workingjubilee, feel free to land when she's satisfied.

@NobodyXu
Rename OnceCell to OnceLock
11d214d 
Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
@NobodyXu
Use Option instead of OnceLock for falliable initialization
3b7f2ef 
The function could have exclusive access to the data, so it does not
need any synchronization.

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
@NobodyXu
Use Once::call_once instead of call_once_force
32cf005 
Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
@NobodyXu
Remove use of one unsafe fn
ec01567 
Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
workingjubilee
workingjubilee approved these changes Jul 14, 2024
View reviewed changes
Copy link
Member

@workingjubilee workingjubilee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this looks much better.

src/parallel/job_token.rs
Comment on lines +187 to +192
let helper_thread = if let Some(thread) = self.helper_thread.as_ref() {
thread
} else {
self.helper_thread
.insert(HelperThread::new(self.jobserver)?)
};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Much nicer.

@NobodyXu NobodyXu merged commit af4c928 into main Jul 15, 2024
50 checks passed
@NobodyXu NobodyXu deleted the fx branch July 15, 2024 01:53
@github-actions github-actions bot mentioned this pull request Jul 15, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@workingjubilee workingjubilee workingjubilee approved these changes

@thomcc thomcc Awaiting requested review from thomcc

@ChrisDenton ChrisDenton Awaiting requested review from ChrisDenton

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Cyclic package dependency with certain package features set
3 participants
@NobodyXu @thomcc @workingjubilee